lint: ensure all files are owned by someone #7937
Conversation
googlebot
added
the
cla: yes
tools/gulp/tasks/lint.ts
Outdated
| function isOwnedBy(path: string, ownedPath: IMinimatch) { | ||
| // If the owned path ends with `**` its safe to eliminate whole directories. | ||
| if (statSync(path).isFile() || ownedPath.pattern.endsWith('**')) { | ||
| return ownedPath.match('/' + path); |
There was a problem hiding this comment.
AFAIK match(someStr) ends up creating a Regex under the hood. This could be clearer and faster using indexOf.
There was a problem hiding this comment.
I was originally extracting the regex and working with that, but the matchBase: true didn't seem to be coded into the regex. Anyways I think it caches the regex under the hood instead of recompiling it every time, so it should be fine.
tools/gulp/tasks/lint.ts
Outdated
| /** Check if the given path is owned by the given owned path matcher. */ | ||
| function isOwnedBy(path: string, ownedPath: IMinimatch) { | ||
| // If the owned path ends with `**` its safe to eliminate whole directories. | ||
| if (statSync(path).isFile() || ownedPath.pattern.endsWith('**')) { |
There was a problem hiding this comment.
nit: Might be better to move the ownedPath.pattern.endsWith('**') in front since it exits faster, because it doesn't have to hit the file system.
tools/gulp/tasks/lint.ts
Outdated
| .concat(ignoreOwners.map(path => new Minimatch(path, {dot: true, matchBase: true}))); | ||
|
|
||
| for (let paths = getChildPaths('.'); paths.length;) { | ||
| paths = Array.prototype.concat(...paths |
There was a problem hiding this comment.
You should be able to get rid of the Array.prototype.concat using a spread: paths = [...paths.filter].
There was a problem hiding this comment.
I think thats the equivalent of slice(...paths) not concat(...paths)
| // Trim lines. | ||
| .map(line => line.trim()) | ||
| // Remove empty lines and comments. | ||
| .filter(line => line && !line.startsWith('#')) |
There was a problem hiding this comment.
This won't handle cases like /some/path owner1 owner2 #owner3.
There was a problem hiding this comment.
I think it will? it splits on white space and just cares about the first piece
tools/gulp/tasks/lint.ts
Outdated
| const ownersFilePath = '.github/CODEOWNERS'; | ||
|
|
||
| /** Paths that should be ignored when checking for owners coverage. */ | ||
| const ignoreOwners = ['/node_modules/**', '/dist/**', '/.git/**', '/.idea/**']; |
There was a problem hiding this comment.
This doesn't really cover all the paths (e.g. there's /.vscode which should also be excluded). Consider populating the list from the .gitignore instead?
tools/gulp/tasks/lint.ts
Outdated
| // Report an error for any files that didn't match any owned paths. | ||
| .filter(path => { | ||
| if (statSync(path).isFile()) { | ||
| console.error(red(`No code owner found for "${path}".`)); |
There was a problem hiding this comment.
If you want to make this look like a Gulp task out, you can use the gulp-util.
tools/gulp/tasks/lint.ts
Outdated
| // Remove empty lines and comments. | ||
| .filter(line => line && !line.startsWith('#')) | ||
| // Split off just the path glob and turn it into a regex. | ||
| .map(line => new Minimatch(line.split(/\s+/)[0], {dot: true, matchBase: true})) |
There was a problem hiding this comment.
You can avoid having to repeat the Minimatch constructor config by concat-ing the ignoreOwners earlier:
readFileSync(ownersFilePath, 'utf8').split('\n')
// Trim lines.
.map(line => line.trim())
// Remove empty lines and comments.
.filter(line => line && !line.startsWith('#'))
// Take out the file pattern.
.map(line => line.split(/\s+/)[0])
// Add in the paths we're ignoring.
.concat(ignoreOwners)
// Turn the paths into regex.
.map(line => new Minimatch(line, {dot: true, matchBase: true}))
|
comments addressed |
crisbeto
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.